[Merged by Bors] - Documenting `UniformBuffer`, `DynamicUniformBuffer`, `StorageBuffer` and `DynamicStorageBuffer`. #5223

bzm3r · 2022-07-06T02:05:35Z

Objective

Documents the UniformBuffer, DynamicUniformBuffer, StorageBuffer and DynamicStorageBuffer render resources.

Solution

I looked through Discord discussion on these structures, and found a comment to be particularly helpful, in the general discussion around encase. Other resources I have used are documented here: https://discord.com/channels/691052431525675048/968333504838524958/991195474029715520

Co-authored-by: Robert Swain <robert.swain@gmail.com>

…w being merged. Co-authored-by: Robert Swain <robert.swain@gmail.com>

…ce memory" Co-authored-by: Robert Swain <robert.swain@gmail.com>

Co-authored-by: Robert Swain <robert.swain@gmail.com>

alice-i-cecile · 2022-07-06T02:07:16Z

I can't comment on the accuracy of these particular docs but the writing is excellent!

alice-i-cecile · 2022-07-06T13:43:46Z

Ping @superdump for review.

…documentation for StorageBuffer.

alice-i-cecile

IMO you should remove the "helper structure" terminology from the start of these docs :) It doesn't clarify things, and takes up space in the most important part of the description.

…mic storage buffer docs

superdump · 2022-07-09T08:24:11Z

crates/bevy_render/src/render_resource/storage_buffer.rs

+/// can store much larger data than uniform buffers, which are best suited to relatively small data.
+///
+/// Storage buffers can store runtime-sized arrays, but only if they are the last field in a structure. To store a
+/// runtime-sized array of data, use [`DynamicStorageBuffer`](crate::render_resource::DynamicStorageBuffer) instead.


I think this needs to be improved. It’s complicated though so I’ll have to have a go at a suggestion another time.

It's something like: if you want a runtime-sized array that you will index into from within your shaders, you can have one at the end of the inner type of a storage buffer. If you want multiple storage buffers of the same inner type and you will bind them in separate draw commands, you can use the dynamic storage buffer and pass the dynamic offsets when binding the buffer.

Dynamic storage buffers aren't really for runtime-sized arrays specifically. More for separating bindings due to some constraint or other or it just being more convenient for some reason. I kind of expect in practice it to be more desirable to use one storage buffer binding with a trailing runtime-sized array than to use dynamic offsets. This is because you can't batch draw calls together if you have to pass different dynamic offsets for the bindings.

superdump · 2022-07-09T08:26:48Z

crates/bevy_render/src/render_resource/storage_buffer.rs

+/// can store much larger data than uniform buffers, which are best suited to relatively small data. Dynamic storage buffers
+/// are particularly well-suited to storing like a Rust-vector (wgpu's "runtime-sized arrays") and have a
+/// [`push`](crate::render_resource::DynamicStorageBuffer::push) method, unlike
+/// [`StorageBuffer`](crate::render_resource::StorageBuffer).


I don’t agree with this. I’ll try to make a suggestion soon.

I'd probably rather say that StorageBuffer is well-suited to storing a single runtime-sized array like a Rust Vec, unlike UniformBuffer which are limited size (minimum guaranteed size is 16kB) and can only contain fixed-size arrays.

superdump · 2022-07-09T08:29:42Z

crates/bevy_render/src/render_resource/storage_buffer.rs

+/// The contained data is stored in system RAM. [`write_buffer`](crate::render_resource::DynamicStorageBuffer::write_buffer)
+/// queues copying of the data from system RAM to VRAM. Dynamic storage buffers must conform to
+/// [std430 alignment/padding requirements], which is automatically enforced by this structure. If data does not need to
+/// be automatically padded or aligned, consider using [`BufferVec`](crate::render_resource::BufferVec).


Additionally, each item pushed gets aligned to the required dynamic offset alignment for the wgpu backend and GPU vendor so that the byte offset provided in the binding call is valid.

@superdump To address this, I wrote down now:

Dynamic storage buffers must conform to [std430 alignment/padding requirements]; whenever data is pushed into this structure, it is automatically aligned to these requirements.

Is that okay?

I don't think dynamic offsets are part of std430. std430 is about the alignment of data within the binding. Dynamic offsets are about the alignment of the binding itself.

superdump · 2022-07-09T08:31:49Z

crates/bevy_render/src/render_resource/uniform_buffer.rs

+/// Uniform buffers are available to shaders on a read-only basis. Uniform buffers are commonly used to make available to shaders
+/// parameters that are constant during shader execution, and are best used for data that is relatively small in size. For
+/// larger data, or data that must be made accessible to shaders on a read-write basis, consider
+/// [`StorageBuffer`](crate::render_resource::StorageBuffer).


Note that WebGL2 does not support storage buffers. Alternatives for large amounts of data there are vertex/instance buffers, or data textures, depending on what is the most appropriate for the use case.

superdump · 2022-07-09T08:33:11Z

crates/bevy_render/src/render_resource/uniform_buffer.rs

+/// The contained data is stored in system RAM. [`write_buffer`](crate::render_resource::UniformBuffer::write_buffer) queues
+/// copying of the data from system RAM to VRAM. Data in uniform buffers must follow [std140 alignment/padding requirements],
+/// which is automatically enforced by this structure. Per the WGPU spec, uniform buffers cannot store runtime-sized array (vectors), or structures with fields that are vectors.
+/// If this is required, consider [`DynamicUniformBuffer`](crate::render_resource::DynamicUniformBuffer).


There is a bit of a hack one can do to fake runtime sized arrays but I suppose it should be documented in an advanced section of the book rather than here.

Okay, so I will leave this section as is. I know you have described this hack to me on the Discord, and it makes sense! I will leave a link here: https://discord.com/channels/691052431525675048/866787577687310356/991373107065532538

And I will also leave a link in the rendering documentation thread on Discord. (see: https://discord.com/channels/691052431525675048/968333504838524958)

crates/bevy_render/src/render_resource/storage_buffer.rs

crates/bevy_render/src/render_resource/uniform_buffer.rs

superdump · 2022-07-13T09:21:21Z

I'm starting to feel like this is the wrong place for all the additional information about the other choices. Maybe it would be better to just write what each is specifically suitable for, and then just list the other possibilities without justification. Then we should have a more informative section in the bevy book that helps deciding which to use. It feels complicated in the scope of documentation here. What do you think @alice-i-cecile ?

alice-i-cecile · 2022-07-18T18:51:59Z

I'm in agreement with @superdump here; I think we can strip this down a bit (but keep basic cross-links) and then do a better overview at the module or book level.

Co-authored-by: Robert Swain <robert.swain@gmail.com>

…t of links to other data storage options at end of docstring fro each particular data storage option

bzm3r · 2022-07-20T02:12:33Z

@superdump @alice-i-cecile sorry, this is a bit late, but I incorporated rob's suggestions and I also stripped down the docs to avoid discussing what the use cases of the other options are. Instead, I've stuck to discussing only the pros/cons of the specific option in question, and provided a set of links at the end to the other options. For example, for at the end of the docs for storage buffer:

/// Other options for storing GPU-accessible data are:
/// * [`DynamicStorageBuffer`](crate::render_resource::DynamicStorageBuffer)
/// * [`UniformBuffer`](crate::render_resource::UniformBuffer)
/// * [`DynamicUniformBuffer`](crate::render_resource::DynamicUniformBuffer)
/// * [`BufferVec`](crate::render_resource::BufferVec)
/// * [`Texture`](crate::render_resource::Texture)

Also, CI is failing because clippy is detecting WebGL2 as something that should go in backticks (see: https://discord.com/channels/691052431525675048/743559241461399582/996472353594822696). I think this is a spurious fail, and can be ignored?

superdump

Looks good to me. @alice-i-cecile ?

alice-i-cecile

bors r+

…and `DynamicStorageBuffer`. (#5223) # Objective Documents the `UniformBuffer`, `DynamicUniformBuffer`, `StorageBuffer` and `DynamicStorageBuffer` render resources. ## Solution I looked through Discord discussion on these structures, and found [a comment](https://discord.com/channels/691052431525675048/953222550568173580/956596218857918464) to be particularly helpful, in the general discussion around encase. Other resources I have used are documented here: https://discord.com/channels/691052431525675048/968333504838524958/991195474029715520 Co-authored-by: Brian Merchant <bhmerchant@gmail.com>

alice-i-cecile · 2022-07-20T13:48:37Z

@bzm3r get CI passing with either an ignore or backticks (mild preference for ignore) and I'll merge this.

bors · 2022-07-20T13:49:52Z

Build failed:

ci

…ckticks

alice-i-cecile · 2022-07-20T17:06:32Z

bors retry

…and `DynamicStorageBuffer`. (#5223) # Objective Documents the `UniformBuffer`, `DynamicUniformBuffer`, `StorageBuffer` and `DynamicStorageBuffer` render resources. ## Solution I looked through Discord discussion on these structures, and found [a comment](https://discord.com/channels/691052431525675048/953222550568173580/956596218857918464) to be particularly helpful, in the general discussion around encase. Other resources I have used are documented here: https://discord.com/channels/691052431525675048/968333504838524958/991195474029715520 Co-authored-by: Brian Merchant <bhmerchant@gmail.com>

bors · 2022-07-20T17:46:30Z

Pull request successfully merged into main.

Build succeeded:

…and `DynamicStorageBuffer`. (bevyengine#5223) # Objective Documents the `UniformBuffer`, `DynamicUniformBuffer`, `StorageBuffer` and `DynamicStorageBuffer` render resources. ## Solution I looked through Discord discussion on these structures, and found [a comment](https://discord.com/channels/691052431525675048/953222550568173580/956596218857918464) to be particularly helpful, in the general discussion around encase. Other resources I have used are documented here: https://discord.com/channels/691052431525675048/968333504838524958/991195474029715520 Co-authored-by: Brian Merchant <bhmerchant@gmail.com>

Brian Merchant and others added 10 commits May 6, 2022 13:30

Documenting BufferVec.

11727c5

Replace RenderDevice by GPU.

9ef6d24

Co-authored-by: Robert Swain <robert.swain@gmail.com>

State Pod as the direct requirement; and reflect reality of encase no…

5ffd580

…w being merged. Co-authored-by: Robert Swain <robert.swain@gmail.com>

Say "system RAM" instead of "host memory" and "VRAM" instead of "devi…

da1f666

…ce memory" Co-authored-by: Robert Swain <robert.swain@gmail.com>

remove repetition of "data"

e407b97

Co-authored-by: Robert Swain <robert.swain@gmail.com>

run cargo fmt

c50f80c

Merge branch 'main' of github.com:bevyengine/bevy

1dab056

Add documentation for UniformBuffer and DynamicUniformBuffer.

b974929

run cargo fmt

8a49be6

Merge branch 'bevyengine:main' into main

919c444

alice-i-cecile added C-Docs An addition or correction to our documentation A-Rendering Drawing game state to the screen labels Jul 6, 2022

bzm3r added 2 commits July 6, 2022 14:07

Clarifications regarding dynamic vs. non-dynamic buffers. Also added …

f77cd5a

…documentation for StorageBuffer.

Merge branch 'main' of github.com:bzm3r/bevy

ecfeac0

alice-i-cecile requested changes Jul 7, 2022

View reviewed changes

bzm3r marked this pull request as draft July 7, 2022 22:06

bzm3r changed the title ~~Documenting UniformBuffer and DynamicUniformBuffer.~~ Documenting UniformBuffer, DynamicUniformBuffer, StorageBuffer and DynamicStorageBuffer. Jul 7, 2022

bzm3r added 2 commits July 8, 2022 14:04

clarifying docs further.

908bd6a

say 'dynamic storage buffer', instead of only storage buffer, in dyna…

d275294

…mic storage buffer docs

bzm3r marked this pull request as ready for review July 8, 2022 21:09

superdump requested changes Jul 9, 2022

View reviewed changes

addressing suggestions by @superdump (robswain)

d34311f

superdump added this to the Bevy 0.8 milestone Jul 13, 2022

superdump requested changes Jul 13, 2022

View reviewed changes

alice-i-cecile removed this from the Bevy 0.8 milestone Jul 18, 2022

Update crates/bevy_render/src/render_resource/uniform_buffer.rs

e77771e

Co-authored-by: Robert Swain <robert.swain@gmail.com>

bzm3r and others added 6 commits July 19, 2022 18:38

Update crates/bevy_render/src/render_resource/uniform_buffer.rs

8cb0f04

Co-authored-by: Robert Swain <robert.swain@gmail.com>

Update crates/bevy_render/src/render_resource/storage_buffer.rs

d4c1761

Co-authored-by: Robert Swain <robert.swain@gmail.com>

Update crates/bevy_render/src/render_resource/storage_buffer.rs

a109dad

Co-authored-by: Robert Swain <robert.swain@gmail.com>

Update crates/bevy_render/src/render_resource/storage_buffer.rs

d5bf796

Co-authored-by: Robert Swain <robert.swain@gmail.com>

Update crates/bevy_render/src/render_resource/uniform_buffer.rs

4ff1a55

Co-authored-by: Robert Swain <robert.swain@gmail.com>

strip down docs a bit, to avoid repetition, and provide a cohesive se…

fb5001a

…t of links to other data storage options at end of docstring fro each particular data storage option

superdump approved these changes Jul 20, 2022

View reviewed changes

alice-i-cecile approved these changes Jul 20, 2022

View reviewed changes

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jul 20, 2022

allow doc_markdown clippy lint so that we can write WebGL2 without ba…

15f7033

…ckticks

bors bot changed the title ~~Documenting UniformBuffer, DynamicUniformBuffer, StorageBuffer and DynamicStorageBuffer.~~ [Merged by Bors] - Documenting UniformBuffer, DynamicUniformBuffer, StorageBuffer and DynamicStorageBuffer. Jul 20, 2022

bors bot closed this Jul 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Documenting `UniformBuffer`, `DynamicUniformBuffer`, `StorageBuffer` and `DynamicStorageBuffer`. #5223

[Merged by Bors] - Documenting `UniformBuffer`, `DynamicUniformBuffer`, `StorageBuffer` and `DynamicStorageBuffer`. #5223

bzm3r commented Jul 6, 2022 •

edited

Loading

alice-i-cecile commented Jul 6, 2022

alice-i-cecile commented Jul 6, 2022

alice-i-cecile left a comment

superdump Jul 9, 2022

superdump Jul 13, 2022

superdump Jul 9, 2022

superdump Jul 13, 2022

superdump Jul 9, 2022

bzm3r Jul 11, 2022 •

edited

Loading

superdump Jul 13, 2022

superdump Jul 9, 2022

superdump Jul 9, 2022

bzm3r Jul 11, 2022

superdump commented Jul 13, 2022

alice-i-cecile commented Jul 18, 2022

bzm3r commented Jul 20, 2022 •

edited

Loading

superdump left a comment

alice-i-cecile left a comment

alice-i-cecile commented Jul 20, 2022

bors bot commented Jul 20, 2022

alice-i-cecile commented Jul 20, 2022

bors bot commented Jul 20, 2022

[Merged by Bors] - Documenting UniformBuffer, DynamicUniformBuffer, StorageBuffer and DynamicStorageBuffer. #5223

[Merged by Bors] - Documenting UniformBuffer, DynamicUniformBuffer, StorageBuffer and DynamicStorageBuffer. #5223

Conversation

bzm3r commented Jul 6, 2022 • edited Loading

Objective

Solution

alice-i-cecile commented Jul 6, 2022

alice-i-cecile commented Jul 6, 2022

alice-i-cecile left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzm3r Jul 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superdump commented Jul 13, 2022

alice-i-cecile commented Jul 18, 2022

bzm3r commented Jul 20, 2022 • edited Loading

superdump left a comment

Choose a reason for hiding this comment

alice-i-cecile left a comment

Choose a reason for hiding this comment

alice-i-cecile commented Jul 20, 2022

bors bot commented Jul 20, 2022

alice-i-cecile commented Jul 20, 2022

bors bot commented Jul 20, 2022

[Merged by Bors] - Documenting `UniformBuffer`, `DynamicUniformBuffer`, `StorageBuffer` and `DynamicStorageBuffer`. #5223

[Merged by Bors] - Documenting `UniformBuffer`, `DynamicUniformBuffer`, `StorageBuffer` and `DynamicStorageBuffer`. #5223

bzm3r commented Jul 6, 2022 •

edited

Loading

bzm3r Jul 11, 2022 •

edited

Loading

bzm3r commented Jul 20, 2022 •

edited

Loading